[CALCITE-6690] Refactor the Arrow adapter type system #4052
Conversation
|
After this pr, for some data types, we can easily identify their precision and scale such as decimal, time, timestamp |
| LONG(Primitive.LONG), | ||
| BYTE(Primitive.BYTE), | ||
| SHORT(Primitive.SHORT); | ||
| class ArrowFieldType { |
There was a problem hiding this comment.
I think enums are preferable, since they are typechecked by the compiler.
A Class can be anything.
There was a problem hiding this comment.
Later, I may dynamically pass some parameters according to the data type of arrow, which is difficult to achieve through enumeration.
e.g.
new ArrowFieldType(BigDecimal.class,
((ArrowType.Decimal) arrowType).getPrecision(),
((ArrowType.Decimal) arrowType).getScale())
There was a problem hiding this comment.
Why later if you are refactoring the code now?
Do it correctly the first time.
I am suggesting to have 3 arguments for the enum constructor: Class, precision, scale.
There was a problem hiding this comment.
@mihaibudiu Sorry for the late reply, you are right. I looked at the arrow types and it seems that Class, precision, and scale are sufficient.
I will finish this work on the weekend
There was a problem hiding this comment.
@mihaibudiu Hi. I have a question, how to dynamically pass parameters in enumeration class? Precision and scale are not constants for decimal, timestamp and other types.
There was a problem hiding this comment.
I am not sure I understand the question.
There are lots of enum classes with parameters in the codebase.
See https://github.com/apache/calcite/blob/main/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java for example
There was a problem hiding this comment.
@mihaibudiu thanks, done
There was a problem hiding this comment.
The strange thing is that when I don't use an enumeration to pass in the precision, the CLI will not report an error
There was a problem hiding this comment.
What is the CLI?
Sorry. it is CI
| switch (vector.getMinorType()) { | ||
| case TINYINT: | ||
| switch (field.getName()) { | ||
| case "tinyIntField": |
There was a problem hiding this comment.
This is a necessary change? What is the difference between decimalField2 and decimalField?
There was a problem hiding this comment.
Required, the scale of decimalfield and decimalfiled2 are different, one is 2, the other is 3, the main purpose is to capture the accuracy of whether it is effective
|
Sorry for confusing the commit message. I'm not sure if it was my changes that caused the cli to fail. I haven't reproduced the problem locally. |
| @@ -55,9 +45,20 @@ enum ArrowFieldType { | |||
| this.clazz = clazz; | |||
| } | |||
|
|
|||
| ArrowFieldType(Class<?> clazz, int precision, int scale) { | |||
There was a problem hiding this comment.
I think the other constructors should call this one.
There are some values in the type system for "unspecified", maybe these could be reused.
Using 0 may not be always safe.
| @@ -732,8 +732,8 @@ static void initializeArrowState(@TempDir Path sharedTempDir) | |||
| @Test void testFilteredAgg() { | |||
| String sql = "select SUM(SAL) FILTER (WHERE COMM > 400) as SALESSUM from EMP"; | |||
| String plan = "PLAN=EnumerableAggregate(group=[{}], SALESSUM=[SUM($0) FILTER $1])\n" | |||
| + " EnumerableCalc(expr#0..7=[{inputs}], expr#8=[400:DECIMAL(19, 0)], expr#9=[>($t6, $t8)], " | |||
| + "expr#10=[IS TRUE($t9)], SAL=[$t5], $f1=[$t10])\n" | |||
| + " EnumerableCalc(expr#0..7=[{inputs}], expr#8=[CAST($t6):DECIMAL(10, 2)], expr#9=[400.00:DECIMAL(10, 2)], " | |||
There was a problem hiding this comment.
can you also add or modify a test that uses a decimal number with digits after the decimal point? This would exercise the scale.
|
https://issues.apache.org/jira/browse/CALCITE-6690